feat: revoke token chain by consent challenge ID #3932

alnr · 2025-01-30T16:41:25Z

This change adds the ability to revoke token chains by "consent challenge ID".

"Consent sessions"

Each time the user goes through a GET /oauth2/auth?response_type=code&... auth code flow, we persist a new "consent session" to the database.

This is independent of whether the user has previously logged in and/or granted consent, or whether the user was actively asked to grant consent by the consent app. A successful journey through the auth code flow results in a new "consent session".

This consent session is uniquely identified by its "consent challenge ID". This ID is obtained from the GET /admin/oauth2/auth/requests/consent?consent_challenge=... API. Note that it is not the same as the consent_challenge=... query parameter!

Any access and refresh tokens obtained from a token exchange following that particular user journey are bound to that consent session.

We call the totality of all refresh+access tokens derived from a particular consent session a "token chain".

Token revocation

Revoking an access token (AT) is simple: send the AT to /oauth2/revoke and it is revoked. If this AT was birthed from a refresh token (RT), the RT that birthed it is not revoked.

Revoking a refresh token (RT) also revokes assocated access tokens.

What if I want to revoke a complete token chain given only an access token?

Revocation by consent challenge ID

During an authorization code flow, save the consent challenge ID into the access token session data:

GET /admin/oauth2/auth/requests/consent?consent_challenge=abcdef

Response:

{
  "acr": ...,
  "challenge": "G_TIM3XABG14UwIgDoT1DRfipjhC1uix" # <- this is the ID we need
  ...
}

Accept the consent request:

PUT /admin/oauth2/auth/requests/consent/accept?consent_challenge=abcdef
{
  "remember": true,
  "remember_for": 3600,
  "session": {
    "access_token": {
      "ccid": "G_TIM3XABG14UwIgDoT1DRfipjhC1uix"
    }
  },
  ...
}

To revoke the token chain associated with this consent challenge ID, use

POST admin/oauth2/auth/sessions/consent?consent_challenge_id=G_TIM3XABG14UwIgDoT1DRfipjhC1uix

PR notes

The persistence code and much of the test code are pretty bad. We test implementation not behavior. There are wrong abstractions.

I have deleted sdk_test.go because honestly I can't see the point of that whole file.

flow/flow.go

aeneasr

Is the consent challenge id set correctly during the get consent flow? Meaning it won't change after we accept the consent challenge? I believe that was the issue when I looked at it. Maybe an e2e test could bring assurance

internal/httpclient/api/openapi.yaml

consent/sdk_test.go

alnr · 2025-02-05T16:46:18Z

Is the consent challenge id set correctly during the get consent flow? Meaning it won't change after we accept the consent challenge? I believe that was the issue when I looked at it. Maybe an e2e test could bring assurance

Check out this test: https://github.com/ory/hydra/pull/3932/files#diff-6d883efffdabd9715dc9872121018df30a5843c81e25dc6c4af2c3edc13fb21cR1194

alnr · 2025-02-05T17:37:56Z

Is the consent challenge id set correctly during the get consent flow? Meaning it won't change after we accept the consent challenge? I believe that was the issue when I looked at it. Maybe an e2e test could bring assurance

Check out this test: https://github.com/ory/hydra/pull/3932/files#diff-6d883efffdabd9715dc9872121018df30a5843c81e25dc6c4af2c3edc13fb21cR1194

I've now expanded this test to be more explicit about what does and does not get revoked, and directly test the customer use case (store the challenge ID inside the token).

hperl

LGTM!

zepatrik

I don't see a blocker, but let's clarify and in case follow up on these minor comments.

zepatrik · 2025-02-07T12:56:55Z

consent/handler.go

+		return
+	}
+	if consentChallengeID != "" && client != "" {
+		h.r.Writer().WriteError(w, r, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'client' and 'consent_challenge_id' cannot be set at the same time.`)))
 		return
 	}


I think one combination missing here is when consentChallenge and allClients are set, then we'll run into the allClients case but with an empty subject. Not sure what exactly the function does in that case, but please double check and add this validation as a follow-up. Consider checking for valid combinations instead of invalid ones and using a switch statement.

zepatrik · 2025-02-07T13:38:43Z

oauth2/oauth2_auth_code_test.go

@@ -1192,6 +1204,79 @@ func TestAuthCodeWithDefaultStrategy(t *testing.T) {
 				t.Run("strategy=jwt", run("jwt"))
 			})

+			t.Run("case=can revoke token chains with ID obtained from consent requests", func(t *testing.T) {


It would be good to test a real "chain", i.e. len(chain) > 1 here. Especially since we have the graceful refresh, we can get forks in the chain and I'd expect to have all branches revoked as well, right?

zepatrik · 2025-02-07T13:41:12Z

persistence/sql/persister_consent.go

@@ -306,10 +311,6 @@ func (p *Persister) VerifyAndInvalidateConsentRequest(ctx context.Context, verif
 		return nil, errorsx.WithStack(fosite.ErrInvalidRequest.WithDebug(err.Error()))
 	}

-	// We set the consent challenge ID to a new UUID that we can use as a foreign key in the database
-	// without encoding the whole flow.
-	f.ConsentChallengeID = sqlxx.NullString(uuid.Must(uuid.NewV4()).String())


Why exactly do we not update this anymore? I also see it removed in some tests.

alnr self-assigned this Jan 30, 2025

alnr requested review from aeneasr and a team as code owners January 30, 2025 16:41

alnr force-pushed the alnr/revoke-consent-by-id branch from af20e3f to fa2728b Compare January 30, 2025 17:52

aeneasr reviewed Feb 3, 2025

View reviewed changes

flow/flow.go Outdated Show resolved Hide resolved

aeneasr reviewed Feb 3, 2025

View reviewed changes

internal/httpclient/api/openapi.yaml Show resolved Hide resolved

consent/sdk_test.go Show resolved Hide resolved

alnr force-pushed the alnr/revoke-consent-by-id branch from 065a0c6 to 31ccc45 Compare February 5, 2025 17:40

hperl approved these changes Feb 7, 2025

View reviewed changes

feat: revoke consent + token chains by consent challenge ID

069671c

alnr force-pushed the alnr/revoke-consent-by-id branch from 31ccc45 to 069671c Compare February 7, 2025 12:05

alnr enabled auto-merge February 7, 2025 12:05

alnr added this pull request to the merge queue Feb 7, 2025

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 7, 2025

alnr added this pull request to the merge queue Feb 7, 2025

zepatrik reviewed Feb 7, 2025

View reviewed changes

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: revoke token chain by consent challenge ID #3932

feat: revoke token chain by consent challenge ID #3932

alnr commented Jan 30, 2025 •

edited

Loading

aeneasr left a comment

alnr commented Feb 5, 2025

alnr commented Feb 5, 2025

hperl left a comment

zepatrik left a comment

zepatrik Feb 7, 2025

zepatrik Feb 7, 2025

zepatrik Feb 7, 2025

feat: revoke token chain by consent challenge ID #3932

Are you sure you want to change the base?

feat: revoke token chain by consent challenge ID #3932

Conversation

alnr commented Jan 30, 2025 • edited Loading

"Consent sessions"

Token revocation

Revocation by consent challenge ID

PR notes

aeneasr left a comment

Choose a reason for hiding this comment

alnr commented Feb 5, 2025

alnr commented Feb 5, 2025

hperl left a comment

Choose a reason for hiding this comment

zepatrik left a comment

Choose a reason for hiding this comment

zepatrik Feb 7, 2025

Choose a reason for hiding this comment

zepatrik Feb 7, 2025

Choose a reason for hiding this comment

zepatrik Feb 7, 2025

Choose a reason for hiding this comment

alnr commented Jan 30, 2025 •

edited

Loading